fix(monaco): address timing issue in validation patch#67
Conversation
📝 WalkthroughWalkthroughUpdates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
396d028 to
af5de65
Compare
Signed-off-by: John Yanarella <jyanarella@nvidia.com>
af5de65 to
1a5fec0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
patches/monaco-editor.patch (3)
804-806:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrong model event method is invoked (runtime TypeError risk).
Line 805 calls
model.onDidValidateModelVersion(listener), but the added model API isonDidValidateVersion(listener)(see Lines 757-767). This will fail at runtime.Suggested fix
export function onDidValidateModelVersion(model, listener) { - return model.onDidValidateModelVersion(listener); + return model.onDidValidateVersion(listener); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 804 - 806, The wrapper function onDidValidateModelVersion incorrectly calls a non-existent model method; change the call inside onDidValidateModelVersion to invoke the model's actual API onDidValidateVersion(listener) so the wrapper delegates to model.onDidValidateVersion and avoids the runtime TypeError.
241-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Delayer.cancel()now leaves pending promises unresolved.On Line 253,
completionPromiseis nulled without resolving/rejecting it. Any caller awaiting the previoustrigger()promise can hang indefinitely.Suggested fix
cancel() { this.cancelTimeout(); if (this.completionPromise) { - // this.doReject?.(new CancellationError()); + // Treat cancellation as a normal completion to avoid unhandled rejections. + this.doResolve?.(undefined); + this.doResolve = null; + this.doReject = null; this.completionPromise = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 241 - 253, Delayer.cancel() currently clears completionPromise without resolving or rejecting it, which can leave callers awaiting trigger() hanging; restore proper completion by invoking the stored rejection/resolution callback when canceling: in the Delayer.cancel method call this.doReject?.(new CancellationError()) or this.doResolve?.(undefined) as appropriate before setting completionPromise = null so any pending promise from trigger() completes, and ensure you reference the existing symbols this.doReject, this.doResolve, completionPromise, and the trigger() workflow when making the change.
951-962:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTypeScript diagnostics still applies stale markers for newer model versions.
You capture
modelVersionId(Line 943) but do not compare it beforesetModelMarkers(Line 955). If content changes during async validation, stale diagnostics can still be rendered.Suggested fix
if (model.isDisposed()) { return; } // PATCH: Don't apply model markers if the language has changed. (Aligns this behavior with other language services.) if (model.getLanguageId() !== this._selector) { return; } +if (model.getVersionId() !== modelVersionId) { + return; +} editor.setModelMarkers( model, this._selector, diagnostics.map((d) => this._convertDiagnostics(model, d)) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/monaco-editor.patch` around lines 951 - 962, The patch must avoid applying stale diagnostics by verifying the model version before updating markers: before calling editor.setModelMarkers and editor.setModelVersionValidated, compare the captured modelVersionId with the model's current version (e.g., model.getVersionId()) and early-return if they differ; ensure you still check the language mismatch (model.getLanguageId() !== this._selector) first, and only call setModelMarkers(this._selector, diagnostics.map(d => this._convertDiagnostics(model, d))) and editor.setModelVersionValidated(model, modelVersionId) when the modelVersionId matches the model's current version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@patches/monaco-editor.patch`:
- Around line 804-806: The wrapper function onDidValidateModelVersion
incorrectly calls a non-existent model method; change the call inside
onDidValidateModelVersion to invoke the model's actual API
onDidValidateVersion(listener) so the wrapper delegates to
model.onDidValidateVersion and avoids the runtime TypeError.
- Around line 241-253: Delayer.cancel() currently clears completionPromise
without resolving or rejecting it, which can leave callers awaiting trigger()
hanging; restore proper completion by invoking the stored rejection/resolution
callback when canceling: in the Delayer.cancel method call this.doReject?.(new
CancellationError()) or this.doResolve?.(undefined) as appropriate before
setting completionPromise = null so any pending promise from trigger()
completes, and ensure you reference the existing symbols this.doReject,
this.doResolve, completionPromise, and the trigger() workflow when making the
change.
- Around line 951-962: The patch must avoid applying stale diagnostics by
verifying the model version before updating markers: before calling
editor.setModelMarkers and editor.setModelVersionValidated, compare the captured
modelVersionId with the model's current version (e.g., model.getVersionId()) and
early-return if they differ; ensure you still check the language mismatch
(model.getLanguageId() !== this._selector) first, and only call
setModelMarkers(this._selector, diagnostics.map(d =>
this._convertDiagnostics(model, d))) and editor.setModelVersionValidated(model,
modelVersionId) when the modelVersionId matches the model's current version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 080dd354-869a-4628-a0b5-3259afb4be40
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.jsonpatches/monaco-editor.patch
| + // PATCH: Enable workers to communicate that syntax validation is complete for a specific model version | ||
| + const model = editor.getModel(resource); | ||
| + if (!model) { | ||
| + return; |
There was a problem hiding this comment.
Bail out if async validation arrives here after the model is disposed.
| + if (!model) { | ||
| + return; | ||
| + } | ||
| + const modelVersionId = model.getVersionId(); |
There was a problem hiding this comment.
Capture the versionId at the time of the validation request (this mirrors the unpatched behavior of the separate typescript code path).
| + if (model && !model.isDisposed() && model.getLanguageId() === languageId) { | ||
| + // PATCH: Gracefully handle the case where the model changed or was disposed during async validation. | ||
| + if (model && !model.isDisposed() && model.getLanguageId() === languageId && model.getVersionId() === modelVersionId) { | ||
| editor.setModelMarkers(model, languageId, markers); |
There was a problem hiding this comment.
Bail out if the version of the model has advanced since validation starts (this mirrors the unpatched behavior of the separate typescript code path).
|
🎉 This issue has been resolved in version 0.0.9 🎉 |
Summary by CodeRabbit